Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added KPI visualization #4640

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daniellangnet
Copy link
Contributor

@daniellangnet daniellangnet commented Feb 12, 2020

What type of PR is this?

  • Feature

Description

This is a new visualization meant for business KPIs. Ideally, Redash would have a plugin system and some sort of marketplace where this could live. However, since that seems far away, I think it still makes sense to consider adding this visualization to the core codebase, because it seems like this would be one of the most common use-cases for companies using Redash.

Unlike other visualizations, this one is a bit more opinionated in terms of the data it's meant to represent, hence the name "KPI". It has some similarities with the existing counter visualization, but also offers a number of features that wouldn't make sense for the more generic counter:

  • it can show both a current and a target value and calculate the delta automatically as both percentage or absolute numbers
  • it allows configuration for whether the percentage and/or the absolute delta value should be shown, or neither
  • it allows configuration of whether a positive or negative delta is either good or bad
  • it allows custom labels for the target value so entirely different metrics could be shown also

Screenshots

image
image
image

@daniellangnet
Copy link
Contributor Author

I don't think the check that failed has anything to do with my code changes

@kravets-levko
Copy link
Collaborator

Hi @daniellangnet! That check is for code style, don't worry that much 🙂 (bot opened a PR that applies code style rules to your changes, we'll merge it alongside with this PR)

@gabrieldutra
Copy link
Member

@daniellangnet only thing I noticed is that the className="w-100" can be removed from the inputs once this PR is merged with #4788, since 100% width is now the default behavior for them we no longer need it.

@daniellangnet
Copy link
Contributor Author

@gabrieldutra great, thanks for the review and also thanks for your response on the other PR #4837. I'll wait for that one to get merged and will then rebase this branch

@daniellangnet
Copy link
Contributor Author

@gabrieldutra I rebased this on the current master with the separated visualization package and I also removed the className="w-100" from all the input as per your suggestions. Thanks for the review!

@djabhe
Copy link

djabhe commented Aug 24, 2020

hello i have a question. how to add custom visualization in redash? any video tutorial?

@vipulmathur
Copy link
Contributor

vipulmathur commented Oct 8, 2020

Is there a chance that this PR will merge anytime soon @kravets-levko @daniellangnet? What is blocking it? This is just the viz that I would like to use. Would like to have it merged, rather than use it from the PR's branch.

Thanks for this effort @daniellangnet! 🎉

@daniellangnet
Copy link
Contributor Author

@gabrieldutra I just rebased again to fix some issues with the latest master. Do you foresee that this will ever get merged? I understand if the answer is no, but then I could save myself some time and would only maintain our internal fork, as opposed to the split out PR. Thank you

@kravets-levko kravets-levko self-assigned this Oct 25, 2020
@kravets-levko
Copy link
Collaborator

Hi @daniellangnet! Sorry for such a long delay. I plan to review your PR in a next few days. Meanwhile I have a question (just to understand better where do we go): did you take a Counter code as a base to create a new visualization? 'Cause I have a feeling that we could extend existing Counter visualization instead of adding a new one (but it's also possible that it doesn't worth it)

@daniellangnet
Copy link
Contributor Author

Thanks @kravets-levko! I know you guys had a lot going on, so no worries & thank you for your work. You're right that the code was based on the counter viz as a starting point and I thought about whether it would make sense to extend it instead.

If you look at the description of the PR above, I described my reasoning for thinking that a separate visualization would be better here. In short: the KPI viz is a lot more opinionated about what it shows and as a result can do a whole bunch of stuff that perhaps wouldn't make sense to have in a generic counter visualization.

Happy to follow your lead either way! :)

@daniellangnet
Copy link
Contributor Author

This PR suffers from the same Safari issue that I fixed here: #5236
So once this other PR gets merged, I can rebase this PR again and make the same changes here if you like.

@dengc367
Copy link

I need this feature,is it can merge now?

@nathan-protempo
Copy link

Hello, just wondering what happened to this pull request? It's been stalled for a year, which seems a shame as @daniellangnet put in the effort to create it and this would be a useful addition to the product.

@antoinerrr
Copy link

That would be nice to have...

@guidopetri
Copy link
Contributor

@daniellangnet , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

@guidopetri
Copy link
Contributor

@viettran97118 I've reopened the PR, but as mentioned, it still needs some work. Let me know if you hove any questions, but the main things that need to be done are:

  • merge master into this branch
  • resolve merge conflicts

Once this is done, we can go through review and merge it in.

@viettran97118
Copy link

Hey, I remind this PR. I waiting for merged this feature.

@guidopetri
Copy link
Contributor

@viettran97118 thanks for the reminder. I think we slightly misunderstood each other. I need you to update the PR so that it can be merged in after your updates.

@viettran97118
Copy link

Yes I am willing to do this. I have ideas the shame with this PR.
KPI (Key Performance Indicator) cards in Power BI are a great way to visualize and display important metrics in a clear and easy to understand way. I want to show more than just one metrics of counter chart. In chart, i want show target KPI, current value and gap value. So, have many ideas for KPI card.

@guidopetri
Copy link
Contributor

That sounds great to me!

@antonborgstrom
Copy link

antonborgstrom commented Sep 18, 2024

@viettran97118 and @guidopetri Can you please review and merge this PR – we also would appreciate this feature very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.